feat: Decouple disk wiping from LSO installation#825
Merged
openshift-merge-bot[bot] merged 1 commit intoMay 15, 2026
Merged
Conversation
akrzos
requested review from
mcornea
and removed request for
jtaleric and
rsevilla87
📝 WalkthroughWalkthroughThis PR refactors the local storage configuration model to separate disk preparation concerns from Local Storage Operator installation. Boolean configuration flags are removed in favor of device-list-based triggers for Ignition-driven disk setup, and a new ChangesLocal Storage Configuration Refactoring: Separate Disk Preparation from LSO Installation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml`:
- Line 218: The when condition uses raw vars and can mis-evaluate string values;
update the condition to coerce operands to booleans by applying the | bool
filter to migrate_monitoring, apply_cluster_monitoring_config and setup_lso so
the expression becomes: (migrate_monitoring | bool) or
((apply_cluster_monitoring_config | bool) and (setup_lso | bool)); this ensures
CLI-passed strings like "false" are treated correctly.
In `@ansible/roles/sno-post-cluster-install/tasks/main.yml`:
- Line 165: The when condition uses string-truthiness for
apply_cluster_monitoring_config and setup_lso; change it to use explicit boolean
coercion by adding the Jinja bool filter so the condition reads like: when:
apply_cluster_monitoring_config | bool and setup_lso | bool; this matches
adjacent checks (e.g., sno_metal3 | bool, setup_minio | bool) and prevents
extra-var string "false" from evaluating as true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 772ed91e-493a-476a-b3e0-1336288f254a
📒 Files selected for processing (10)
ansible/roles/configure-local-storage/defaults/main.ymlansible/roles/configure-local-storage/tasks/main.ymlansible/roles/configure-local-storage/templates/controlplane.ign.j2ansible/roles/mno-post-cluster-install/defaults/main/main.ymlansible/roles/mno-post-cluster-install/tasks/main.ymlansible/roles/mno-post-cluster-install/templates/localvolume-lvm.yml.j2ansible/roles/sno-post-cluster-install/defaults/main/main.ymlansible/roles/sno-post-cluster-install/tasks/main.ymldocs/local-storage.mddocs/odf.md
akrzos
force-pushed
the
decouple_disk_wipe_from_lso
branch
from
76180aa to
41eb38b
Compare
Member
Author
|
/test deploy-mno |
Disk preparation (wiping, partitioning, LVM setup) is now triggered by populating device lists alone, without requiring a separate boolean. LSO installation is controlled independently by the new setup_lso toggle, following the established setup_odf/setup_minio pattern. This enables three install paths: - Just install a cluster (default) - Install + wipe disks (populate device lists, no LSO) - Install + LSO (setup_lso: true, with or without disk wiping) Removes controlplane_localstorage_configuration and worker_localstorage_configuration variables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
akrzos
force-pushed
the
decouple_disk_wipe_from_lso
branch
2 times, most recently
from
41eb38b to
12f0b9e
Compare
Member
Author
|
Rebased... /test deploy-mno |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ansible/roles/mno-post-cluster-install/tasks/main.yml (1)
177-177:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against undefined device list variables in LocalVolume creation.
Lines 177 and 186 use the same device list variables without
| default([]). While these tasks are inside thesetup_lso | boolblock, they will still fail if the device list variables are undefined.🛡️ Proposed fix to add safe defaults
- name: Create localvolume-lvm resource - when: (controlplane_localstorage_lvm_devices | length > 0) or (worker_localstorage_lvm_devices | length > 0) + when: (controlplane_localstorage_lvm_devices | default([]) | length > 0) or (worker_localstorage_lvm_devices | default([]) | length > 0) shell: KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc apply -f {{ bastion_cluster_config_dir }}/localstorage/localvolume-lvm.yml - name: Create localvolume-disk resource - when: (controlplane_localstorage_disk_devices | length > 0) or (worker_localstorage_disk_devices | length > 0) + when: (controlplane_localstorage_disk_devices | default([]) | length > 0) or (worker_localstorage_disk_devices | default([]) | length > 0) shell: KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc apply -f {{ bastion_cluster_config_dir }}/localstorage/localvolume-disk.ymlAlso applies to: 186-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ansible/roles/mno-post-cluster-install/tasks/main.yml` at line 177, The when conditions referencing controlplane_localstorage_lvm_devices and worker_localstorage_lvm_devices are not guarded against undefined values; update the LocalVolume creation tasks in main.yml to use safe defaults by appending | default([]) to both variables wherever they are used in the when expressions (e.g., the conditions around the LocalVolume tasks that currently check (controlplane_localstorage_lvm_devices | length > 0) or (worker_localstorage_lvm_devices | length > 0)); ensure you apply the same | default([]) pattern to the duplicate occurrence later (the second when at the other LocalVolume task) so undefined lists evaluate as empty lists instead of causing failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml`:
- Line 164: Update the task's when condition to defensively handle undefined
device lists by applying the Jinja2 default filter to each variable used in the
expression: replace checks like controlplane_localstorage_lvm_devices | length >
0 and controlplane_localstorage_disk_devices | length > 0 with
controlplane_localstorage_lvm_devices | default([]) | length > 0 and
controlplane_localstorage_disk_devices | default([]) | length > 0; do the same
for the worker variants (worker_localstorage_lvm_devices and
worker_localstorage_disk_devices) wherever similar when conditions appear so the
role safely runs even if those vars are unset.
---
Outside diff comments:
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml`:
- Line 177: The when conditions referencing
controlplane_localstorage_lvm_devices and worker_localstorage_lvm_devices are
not guarded against undefined values; update the LocalVolume creation tasks in
main.yml to use safe defaults by appending | default([]) to both variables
wherever they are used in the when expressions (e.g., the conditions around the
LocalVolume tasks that currently check (controlplane_localstorage_lvm_devices |
length > 0) or (worker_localstorage_lvm_devices | length > 0)); ensure you apply
the same | default([]) pattern to the duplicate occurrence later (the second
when at the other LocalVolume task) so undefined lists evaluate as empty lists
instead of causing failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38790a4f-b8e3-4d7b-bdda-70ee587a75e6
📒 Files selected for processing (10)
ansible/roles/configure-local-storage/defaults/main.ymlansible/roles/configure-local-storage/tasks/main.ymlansible/roles/configure-local-storage/templates/controlplane.ign.j2ansible/roles/mno-post-cluster-install/defaults/main/main.ymlansible/roles/mno-post-cluster-install/tasks/main.ymlansible/roles/mno-post-cluster-install/templates/localvolume-lvm.yml.j2ansible/roles/sno-post-cluster-install/defaults/main/main.ymlansible/roles/sno-post-cluster-install/tasks/main.ymldocs/local-storage.mddocs/odf.md
✅ Files skipped from review due to trivial changes (1)
- docs/odf.md
🚧 Files skipped from review as they are similar to previous changes (6)
- ansible/roles/sno-post-cluster-install/tasks/main.yml
- ansible/roles/mno-post-cluster-install/templates/localvolume-lvm.yml.j2
- ansible/roles/configure-local-storage/tasks/main.yml
- ansible/roles/configure-local-storage/templates/controlplane.ign.j2
- docs/local-storage.md
- ansible/roles/configure-local-storage/defaults/main.yml
|
|
||
| - name: Label control-plane nodes for localstorage | ||
| when: controlplane_localstorage_configuration | ||
| when: controlplane_localstorage_lvm_devices | length > 0 or controlplane_localstorage_disk_devices | length > 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find definitions of device list variables used in conditionals
# Search for variable definitions
rg -n 'controlplane_localstorage_lvm_devices|controlplane_localstorage_disk_devices|worker_localstorage_lvm_devices|worker_localstorage_disk_devices' \
--type yaml \
-g '!**/tasks/**' \
-g '**/defaults/**' -g '**/vars/**' -g '**/group_vars/**' -g '**/host_vars/**'Repository: redhat-performance/jetlag
Length of output: 462
🏁 Script executed:
# Check the mno-post-cluster-install role structure and defaults
find ansible/roles/mno-post-cluster-install -type f -name "*.yml" | head -20Repository: redhat-performance/jetlag
Length of output: 246
🏁 Script executed:
# Check if defaults exist in mno-post-cluster-install role
cat ansible/roles/mno-post-cluster-install/defaults/main.yml 2>/dev/null || echo "No defaults file"Repository: redhat-performance/jetlag
Length of output: 86
🏁 Script executed:
# Check role dependencies in mno-post-cluster-install
cat ansible/roles/mno-post-cluster-install/meta/main.yml 2>/dev/null || echo "No meta file"Repository: redhat-performance/jetlag
Length of output: 82
🏁 Script executed:
# Show context around lines 164 and 170 in the actual task file
sed -n '155,175p' ansible/roles/mno-post-cluster-install/tasks/main.ymlRepository: redhat-performance/jetlag
Length of output: 1087
🏁 Script executed:
# Check the defaults files in mno-post-cluster-install
echo "=== defaults/main/main.yml ===" && cat ansible/roles/mno-post-cluster-install/defaults/main/main.yml && echo -e "\n=== defaults/main/odf.yml ===" && cat ansible/roles/mno-post-cluster-install/defaults/main/odf.ymlRepository: redhat-performance/jetlag
Length of output: 3723
🏁 Script executed:
# Also verify how Ansible loads defaults from subdirectories
# Check if these variables are referenced elsewhere in the mno-post-cluster-install role
rg 'controlplane_localstorage_lvm_devices|controlplane_localstorage_disk_devices|worker_localstorage_lvm_devices|worker_localstorage_disk_devices' ansible/roles/mno-post-cluster-install/ -A 2Repository: redhat-performance/jetlag
Length of output: 3105
🏁 Script executed:
# Check how these roles are typically used together - look for playbooks
find . -name "*.yml" -path "*/playbooks/*" -o -path "*/site.yml" -o -name "site.yml" | head -10Repository: redhat-performance/jetlag
Length of output: 51
🏁 Script executed:
# Search for playbook files that include mno-post-cluster-install
rg 'mno-post-cluster-install' --type yaml -A 5 -B 5 | head -50Repository: redhat-performance/jetlag
Length of output: 1960
🏁 Script executed:
# Check for README or documentation about role dependencies
find ansible/roles/mno-post-cluster-install -name "README*" -o -name "*.md" | xargs cat 2>/dev/nullRepository: redhat-performance/jetlag
Length of output: 51
🏁 Script executed:
# Also check configure-local-storage for any documentation
find ansible/roles/configure-local-storage -name "README*" -o -name "*.md" | xargs cat 2>/dev/null | head -50Repository: redhat-performance/jetlag
Length of output: 51
Add defensive defaults to guard against undefined device list variables.
While controlplane_localstorage_lvm_devices, controlplane_localstorage_disk_devices, worker_localstorage_lvm_devices, and worker_localstorage_disk_devices are defined with empty list defaults in the configure-local-storage role and run before this role in standard playbooks, adding | default([]) filters provides better robustness if this role is ever run independently or if variables are cleared.
Suggested fix
- name: Label control-plane nodes for localstorage
- when: controlplane_localstorage_lvm_devices | length > 0 or controlplane_localstorage_disk_devices | length > 0
+ when: (controlplane_localstorage_lvm_devices | default([]) | length > 0) or (controlplane_localstorage_disk_devices | default([]) | length > 0)
shell: |
KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc label no --overwrite {{ item }} localstorage=true
loop: "{{ groups['controlplane'] }}"
- name: Label worker nodes for localstorage
- when: worker_localstorage_lvm_devices | length > 0 or worker_localstorage_disk_devices | length > 0
+ when: (worker_localstorage_lvm_devices | default([]) | length > 0) or (worker_localstorage_disk_devices | default([]) | length > 0)
shell: |
KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc label no --overwrite {{ item }} localstorage=true
loop: "{{ groups['worker'] }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| when: controlplane_localstorage_lvm_devices | length > 0 or controlplane_localstorage_disk_devices | length > 0 | |
| - name: Label control-plane nodes for localstorage | |
| when: (controlplane_localstorage_lvm_devices | default([]) | length > 0) or (controlplane_localstorage_disk_devices | default([]) | length > 0) | |
| shell: | | |
| KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc label no --overwrite {{ item }} localstorage=true | |
| loop: "{{ groups['controlplane'] }}" | |
| - name: Label worker nodes for localstorage | |
| when: (worker_localstorage_lvm_devices | default([]) | length > 0) or (worker_localstorage_disk_devices | default([]) | length > 0) | |
| shell: | | |
| KUBECONFIG={{ bastion_cluster_config_dir }}/kubeconfig oc label no --overwrite {{ item }} localstorage=true | |
| loop: "{{ groups['worker'] }}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml` at line 164, Update
the task's when condition to defensively handle undefined device lists by
applying the Jinja2 default filter to each variable used in the expression:
replace checks like controlplane_localstorage_lvm_devices | length > 0 and
controlplane_localstorage_disk_devices | length > 0 with
controlplane_localstorage_lvm_devices | default([]) | length > 0 and
controlplane_localstorage_disk_devices | default([]) | length > 0; do the same
for the worker variants (worker_localstorage_lvm_devices and
worker_localstorage_disk_devices) wherever similar when conditions appear so the
role safely runs even if those vars are unset.
Collaborator
|
/lgtm |
Collaborator
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mcornea The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 74a9ea4
into
redhat-performance:main
3 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disk preparation (wiping, partitioning, LVM setup) is now triggered by populating device lists alone, without requiring a separate boolean. LSO installation is controlled independently by the new setup_lso toggle, following the established setup_odf/setup_minio pattern.
This enables three install paths:
Removes controlplane_localstorage_configuration and worker_localstorage_configuration variables.
Summary by CodeRabbit
Refactor
New Features
setup_lsosetting to independently control installation of the Local Storage Operator.Documentation